• Support TypedInput
  • What: This PR introduces a pattern for supporting typed inputs for the payload.
  • Why: Because it is super annoying adding change nodes everywhere!
    • All nodes that accept input to have a typedInput #92
  • How: It works by evaluating node config property/propertyType and adding a temporary prop ui_payload to the msg that is used client side as the value. This should work for other widgets too.
  • NOTE: Adding JSONata might be an alt solution to the whole "support mustache for label / text / title / etc" debate
    • Programmatic (Mustache) Titles & Labels #555
    • which makes me think the temp ui_payload should probably be an object like ui_computed so that we can do the custom label stuff too e.g. { ui_computed: { payload: "evaluated payload", "label": "evaluated label" }

Another avenue may be to utilise the existing mechanism of ui_update and add the evaluated payload into that object 🤔

@Steve-Mcl,
My few cents about your proposal, for what it is worth:

1. I really like the idea of using Jsonata to mimic the Mustache expressions from dashboard D1. It is very powerful and then indeed you don't have to pollute your flows with Template nodes. Cool idea!

2. Personally I am not a fan to move the current ouput msg.payload to msg.ui_reason (in order to be able to keep the original input msg.payload). I know that it is advised that nodes should reuse and manipulate existing input messages (by adding data to it), instead of creating new output messages from scratch. In order to keep the (important) info from the input message. However the state of a ui nodes is mostly build upon multiple input messages. The last message might only contain some (unimportant) ui_update stuff, so not really to reuse such messages. Or do you mean that you reuse the last message stored in the data store perhaps? Because most ui nodes currently only store messages which have a payload, it might indeed be useful to reuse that one...

   Moreover when you use msg.ui_reason your flow again needs extra nodes. Because currently I can attach any kind of node to the output of my notification node, because the payload contains all the required info. When you start using msg.ui_reason I need to add Change nodes to move that reason into the payload again...

3. Only sends msg to Node-RED if show is true, makes sense to me. But could it perhaps be solved by moving the
   clear timer and interval code snippet BEFORE the socket.emit (because the emitting perhaps takes a bit too long)?

4. The default passthrough of messages is indeed something that was confusing for me while implementing my PR for the notification node. So indeed I would disable it.

2. Moreover when you use msg.ui_reason your flow again needs extra nodes. Because currently I can attach any kind of node to the output of my notification node, because the payload contains all the required info. When you start using msg.ui_reason I need to add Change nodes to move that reason into the payload again

The current implementation of the UI node only sends "timeout" or "confirm" etc. What I did here was to move the reason for it closing into ui_reason so that the original payload could stay in payload and therefore be passed back for downstream use. And yes the msg passed back will be the last message sent in to that particular notification node.

The point of ui_reason is purely for routing the output message after it has been dismissed.

Fwiw, I don't mind what it's called. I chose ui______ something to minimise clashes with natural props on the msg.

Another suggestion on the forum was multiple outputs. That would remove the need for any property at all.

E.g output 1-Success, output 2-cancelled etc

3. Only sends msg to Node-RED if show is true, makes sense to me. But could it perhaps be solved by moving the
clear timer and interval code snippet BEFORE the socket.emit (because the emitting perhaps takes a bit too long)?

I've been bitten by timeouts in the past.

I did also move the clear calls above the emit but i could still cause dual messages. The interlock was a final catch all that stopped it happening.

I need to spend time on this before committing either way, and give it the attention it deserves, will quickly comment on:

Another suggestion on the forum was multiple outputs. That would remove the need for any property at all.

Whilst it's clean from the notification node perspective, joining these in order to compute on them is then a pain as you have two async messages.

Another suggestion on the forum was multiple outputs. That would remove the need for any property at all.

Whilst it's clean from the notification node perspective, joining these in order to compute on them is then a pain as you have two async messages.

There would be no need to join. The msg (and its original payload) would be sent to the specific output - zero joins, zero switches - if it comes out of port 1 - success, port 2 - cancelled, etc

I need to spend time on this before committing either way, and give it the attention it deserves, will quickly comment on:

Another suggestion on the forum was multiple outputs. That would remove the need for any property at all.

Whilst it's clean from the notification node perspective, joining these in order to compute on them is then a pain as you have two async messages.

I think the confusion is that early on there was a suggestion to move just the result to a separate output, but I think that has been superseded by the suggestion of sending just to one output of the three, dependent on what is clicked. Effectively including a Switch node inside the Notification node.

Does this PR already include adding a note to the docs and help text to say that empty or 0 timeout disables the timeout?

Does this PR already include adding a note to the docs and help text to say that empty or 0 timeout disables the timeout?

No Colin. No docs done at all - very much PoC at the moment.

Will make a mental note to include that kind of info if/when it evolves into something.

In fact that is actually just something missing from the docs for the existing node, it isn't anything to do with the suggested enhancements.

@joepavitt as discussed recently. This rather large PR (sorry) covers the whole boilerplate setup required for nodes when using typedInputs and dynamic properties.

This to check (not sure i have handled, but my flows are now corrupted by the new features (for want of better words)

• When running this against flows developed on <v1.17.0, ensure typedInputs have a sane default and dont change anything operationally for the user.

I am more than happy to do a 1:1 when you come to review as I realise this is rather large.

@Steve-Mcl merging #1343 has triggered a merge conflict - you okay to sanity check it please?

@Steve-Mcl merging #1343 has triggered a merge conflict - you okay to sanity check it please?

All done. Found and fixed an issue too so e2e tests should pass now.
Also, broke then fixed unit tests - bah!

so this (rather large) piece of work now has conflicts again - bah!

There are some things in this PR that fix things like breaking of widgets after being dynamically updated etc - we should get this is. It is likely going to conflict with #1401 / #1428 and #30 / #1369

With these 3 large PRs hanging around, I am loathe to start some of the other tasks that will probably compound this further.

Thanks Steve, it's a huge PR which is going to take a lot of my time to review properly. I can't promise when I can get to it as there are a lot of fundamental changes here.

I understand Joe.

This may be one that is simply pulled and tested.

Happy to do on a call tomorrow to make this swift(er)

Happy to do on a call tomorrow to make this swift(er)

Let's do that - mind putting something in that suits you please?